-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn on a potentially incorrect HTTP OTLP endpoint #126
Conversation
i think we should just add it automatically, thats the behaviour we have with the otel exporter in the rust library |
I actually don't think we should add anything to the path automagically, since there's nothing preventing people from using whatever custom paths they desire. So if we force-add the path, those people would have no way of configuring their custom URL. Regardless, this might be something to properly specify in the Autometrics 1.0 spec, @hatchan |
I think the spec forces this subpath, because the Go library does exactly this. You can configure the value for specific needs of course, but if you just pass the url like |
I thought it was in the defaults that the OTEL collector has, but I'm not sure about the otel spec itself. I find it pretty odd if a application or library does add something by itself, as @arendjr describes, there is no way of doing something else. |
so one option we can take is where if the user submits just the base URL with the port |
…nt is root and port is default
); | ||
} | ||
|
||
if (urlObj.pathname === "/" && urlObj.port === "4317") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid there's still an edge case here. For instance:
new URL("http://localhost:4317").pathname === "/"
In other words, there will not be a distinction between users providing a URL with or without a path. Ie. "http://localhost:4317"
and "http://localhost:4317/"
are interchangeable. But I don't think that's what we want if we automatically add the default path, since the URL ending with /
contains an explicit path and should not be modified, or otherwise we prevent people from submitting to the root path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch. I'll add an exception for cases when root path /
is explicitly referred to.
Co-authored-by: Arend van Beelen jr. <arendjr@gmail.com>
[TODO]: figure out a test for this 🙂 |
This is a small library quality of life PR that will warn a user of the HTTP OTLP exporter if they pass a URL to the
init
function that does not have the spec-compliant/v1/metrics
endpoint.